Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve React warnings and accessibility issues #1763

Merged
merged 39 commits into from
Feb 5, 2025

Conversation

hexart
Copy link
Contributor

@hexart hexart commented Jan 21, 2025

Description

This PR fixes multiple React warnings, accessibility issues and DOM nesting problems across several components:

  • Add DialogTitle for accessibility in Search component
  • Prevent state updates during render in ChatProfiles component
  • Fix invalid DOM nesting of buttons in ThreadOptions
  • Fix invalid DOM nesting of <h2> and <p> tags in Markdown component
  • Add proper key prop to React Fragments in Messages component
  • Fix invalid DOM nesting of Alert component in Markdown paragraphs

Warning before fix

Screenshots

2025-01-30 11 59 01 2025-01-30 11 58 42 2025-01-30 11 58 27 截屏2025-01-30 11 58 13 截屏2025-01-30 11 57 54 截屏2025-01-30 11 57 09

Fixes

  1. Fixed Accessibility Issues in LeftSidebar/Search.tsx:

    • Added screen reader accessible DialogTitle to Search component
    • Implemented sr-only pattern to maintain visual design
    • Resolved Radix UI Dialog accessibility warning
  2. Fixed React Component State Issues in header/ChatProfiles.tsx:

    • Moved state updates from render to useEffect in ChatProfiles component
    • Added early return check to prevent invalid renders
    • Improved type safety with optional chaining
  3. Optimized Profile Selection Logic in header/ChatProfiles.tsx:

    • Split profile selection into separate effects:
      • Handle initial profile selection
      • Handle invalid profile fallback
    • Removed redundant chatProfiles existence checks
    • Improved error handling and edge cases
  4. Fixed DOM Nesting Issues:

    • Resolved invalid button nesting in LeftSidebar/ThreadOptions.tsx
      • Replace Button element with div element in DropdownMenuTrigger
      • Apply equivalent button styles using buttonVariants function
      • Keep all existing behavior and visual styles intact
      • Maintained original styling and functionality
    • Improved DOM nesting handling in Markdown.tsx component
      • Enhanced block element detection to handle headings and Alert components
      • Implemented comprehensive checks for nested divs and component types
      • Added fallback to div container for proper DOM hierarchy
      • Preserved existing markdown styling and functionality
      • Added type safety checks for React elements
  5. Fixed e2e/command/spec.cy.ts Test Cases:

    • Updated command test to handle whitespace properly

Testing Done

  • Confirmed browser console is clean with no error messages
  • Verified accessibility of Search dialog with screen readers
  • Tested chat profile selection functionality
  • Confirmed thread options and list still work as expected
  • Validated markdown content renders correctly
  • Verified Alert components render properly within markdown content

Types of changes

  • Bug fix (non-breaking change which fixes warnings/issues)
  • Accessibility improvements
  • No new feature added

- Move chatProfile selection logic into useEffect
- Add null checks for config object
- Fix React warning about setState during render"
- Update Markdown component to handle heading elements properly
- Fix HTML validation warnings"
- Fix DOM nesting validation warnings
- Maintain existing styling and layout"
- Add key to React.Fragment in message map function
- Fix React warning about missing keys in list
- Replace Button component with styled DropdownMenuTrigger
- Fix DOM nesting warning for button elements
- Move ThreadOptions outside of SidebarMenuButton
- Fix DOM nesting warnings
- Add proper optional chaining for config access
- Move setChatProfile call to useEffect
- Fix TypeScript warnings about potential undefined values
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. frontend Pertains to the frontend. labels Jan 21, 2025
hexart and others added 5 commits January 21, 2025 13:05
- Make text comparison more resilient by using trim()
- Focus on functional correctness rather than exact formatting
- Maintain existing test coverage while improving reliability"
- Remove unused import 'size' from ChatProfiles
- Add eslint-disable-next-line for Card and Table imports
- Fix lint errors while preserving markdown table functionality
- Ensure ESLint correctly recognizes component usage in table renderer
@dokterbob dokterbob added the review-me Ready for review! label Jan 22, 2025
@dokterbob dokterbob enabled auto-merge January 22, 2025 15:37
auto-merge was automatically disabled January 28, 2025 01:18

Head branch was pushed to by a user without write access

frontend/src/components/Markdown.tsx Outdated Show resolved Hide resolved
frontend/src/components/Markdown.tsx Outdated Show resolved Hide resolved
frontend/src/components/LeftSidebar/ThreadList.tsx Outdated Show resolved Hide resolved
frontend/src/components/Markdown.tsx Outdated Show resolved Hide resolved
- Add early return to prevent unnecessary renders and resource waste
- Handle case when no profile is selected
- Handle case when selected profile becomes invalid
- Remove redundant config.chatProfiles existence checks
@hexart hexart requested a review from willydouhard February 4, 2025 08:44
@sandangel
Copy link
Contributor

sandangel commented Feb 4, 2025

Hi @hexart , there is another issue, which is in #chat-input, I type something then remove all, then the send button is not disabled properly. And users now can send empty messages. I wonder if that can also be fixed in this PR? Or should it be fixed in another PR.

@hexart
Copy link
Contributor Author

hexart commented Feb 4, 2025

there is another issue, which is in #chat-input, I type something then remove all, then the send button is not disabled properly. And users now can send empty messages. I wonder if that can also be fixed in this PR? Or should it be fixed in another PR.

I suggest handling the empty message validation in a separate PR. This current one focuses on DOM/rendering fixes, while input validation is a different concern. I'll create a new PR to address the send button and empty message issues.

@sandangel
Copy link
Contributor

No worry, I submitted a PR to fix the issue.

@willydouhard
Copy link
Collaborator

I am not sure what this section does

 const containsBlockElement = useMemo(
            () =>
              React.Children.toArray(props.children).some((child) => {
                if (!React.isValidElement(child)) return false;
                if (
                  /^h[1-6]$/.test(child.type as string) ||
                  child.type === 'p'
                ) {
                  return true;
                }
                if (
                  child.props?.['data-type'] === 'Alert' ||
                  (typeof child.type === 'function' &&
                    child.type.name === 'AlertComponent')
                ) {
                  return true;
                }
                if (React.isValidElement(child) && child.props?.children) {
                  return React.Children.toArray(child.props.children).some(
                    (grandChild) =>
                      React.isValidElement(grandChild) &&
                      (grandChild.type === 'div' ||
                        (typeof grandChild.type === 'string' &&
                          grandChild.type.toLowerCase() === 'div'))
                  );
                }
                return false;
              }),
            [props.children]

Other changes look good to me but it would be nice to have some comments explaining why we need to know if the paragraph contains certain blocks.

@sandangel
Copy link
Contributor

sandangel commented Feb 4, 2025

@willydouhard As I understand, that is for checking when to use p and when to use div for outer most element. If the elements have h1-h6 or have p or containing Alert or some of the grand children containing div, then the outer element should be div, otherwise it can be p.

@willydouhard
Copy link
Collaborator

Can't we always use a div then instead of adding logic?

@sandangel
Copy link
Contributor

@willydouhard the intention seems to keep the original <p> tag implementation. I think we can just use <div> tag and avoid this complex check. I can submit another PR without this check.

@hexart
Copy link
Contributor Author

hexart commented Feb 5, 2025

Can't we always use a div then instead of adding logic?

While using div everywhere might seem simpler, using semantic HTML tags like <p> when appropriate is important for:

  • Accessibility (screen readers)
  • SEO
  • Document structure clarity

I'll add comments to explain this rationale in the code.

@sandangel
Copy link
Contributor

I don't think SEO is related in the chatbot context.

Can you explain why would div affect screen reader and document structure clarity? I do think this logic affect the maintenance effort of the project. Not sure if the benefit worth adding this complex check.

@hexart
Copy link
Contributor Author

hexart commented Feb 5, 2025

I don't think SEO is related in the chatbot context.

Can you explain why would div affect screen reader and document structure clarity? I do think this logic affect the maintenance effort of the project. Not sure if the benefit worth adding this complex check.

You make great points about the context of our application. You're right that in a chatbot UI, we might be over-engineering by adding complex checks for semantic HTML.

Would you be open to this simpler approach? Something like:

p(props) {
  const commonClassNames = 'leading-7 [&:not(:first-child)]:mt-4 whitespace-pre-wrap break-words';
  return <div {...omit(props, ['node'])} className={commonClassNames} role="paragraph" />;
}

@sandangel
Copy link
Contributor

That totally makes sense. changing "role" to paragraph and avoid this complex logic LGTM.

@sandangel
Copy link
Contributor

Thanks for fixing this. We kind of need this change before we can release with chainlit v2.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Feb 5, 2025
@sandangel
Copy link
Contributor

I guess we can also use role="article" if needed as mentioned here https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/article_role

@hexart
Copy link
Contributor Author

hexart commented Feb 5, 2025

I guess we can also use role="article" if needed as mentioned here https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/article_role

Yes, you're right, already fix it.

Copy link
Collaborator

@willydouhard willydouhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@willydouhard willydouhard added this pull request to the merge queue Feb 5, 2025
Merged via the queue into Chainlit:main with commit a58677b Feb 5, 2025
9 checks passed
@hexart hexart deleted the fix/app-render-warning branch February 5, 2025 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Pertains to the frontend. review-me Ready for review! size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants